Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] grapheme_str_split function #13580

Closed
wants to merge 1 commit into from

Conversation

youkidearitai
Copy link
Contributor

I noticed that PHP does not have a grapheme cluster based str_split function. So I created the grapheme_str_split function.
(PCRE can handle grapheme cluster, It can be processed with \X.)
This feature will allow you to correctly handle emoji and Variation Selectors.

I would like to discuss it internal and submit an RFC.

@youkidearitai youkidearitai force-pushed the grapheme_str_split branch 4 times, most recently from 1d9a40a to c9ab75f Compare March 6, 2024 00:36
@youkidearitai
Copy link
Contributor Author

I create an wiki for grapheme_str_split function. Please see:
https://wiki.php.net/rfc/grapheme_str_split

@youkidearitai youkidearitai changed the title [PoC][RFC][WIP] grapheme_str_split function [RFC] grapheme_str_split function Mar 9, 2024
Copy link
Contributor

@Ayesh Ayesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, I left a picked a few nits.
Please also add tests for the split-length and UTF-8 checks.

ext/intl/grapheme/grapheme_string.c Outdated Show resolved Hide resolved
ext/intl/grapheme/grapheme_string.c Outdated Show resolved Hide resolved
@medabkari
Copy link

@youkidearitai Since this RFC is in the Voting phase, I think it should be removed from the Draft section or am i wrong?

@youkidearitai
Copy link
Contributor Author

This an RFC is accepted. Change to normal pull request.

@youkidearitai youkidearitai marked this pull request as ready for review April 10, 2024 00:26
@youkidearitai
Copy link
Contributor Author

@medabkari Thank you! I'm wrong. fixed.

@devnexen
Copy link
Member

Thanks, you need to rebase from last changes on master.

@devnexen
Copy link
Member

tip: once CI all green, please squash your commits into 1. If you become committer, you ll have to do it anyway ;)

I noticed that PHP does not have a grapheme cluster based str_split function.
So I created the grapheme_str_split function.

This feature will allow you to correctly handle emoji
and variable selectors.

Co-authored-by: Ayesh Karunaratne <Ayesh@users.noreply.github.com>
@youkidearitai
Copy link
Contributor Author

tip: once CI all green, please squash your commits into 1. If you become committer, you ll have to do it anyway ;)

Thanks, squashed my commits.

Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@devnexen devnexen closed this in 44e8301 Apr 10, 2024
@devnexen
Copy link
Member

Thank for your work !

@youkidearitai youkidearitai deleted the grapheme_str_split branch April 10, 2024 19:15
@youkidearitai
Copy link
Contributor Author

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants